-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨Finalizer helper library #1481
✨Finalizer helper library #1481
Conversation
Hi @rashmigottipati. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
e4d01c8
to
97f97b2
Compare
1736ee3
to
eb23655
Compare
eb23655
to
2484830
Compare
9ae1fd8
to
e362644
Compare
@rashmigottipati: GitHub didn't allow me to request PR reviews from the following users: jmrodri. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
5e3fa36
to
6d5a162
Compare
@joelanford thanks for the review! Addressed the latest set of comments. Could you please take another look? |
I think the last question I have is whether we want to split the Without that, callers using CRDs with the status subresource will likely need to call both On the otherhand, an API with three return values starts to get into "is this the right API?" territory. |
@joelanford I agree; something like OperationResult would be useful here. |
+1 @estroz. Are you thinking of using that directly or adding a separate type in the finalizer package that conveys just what a finalizer would need? I vote the latter. If you agree, I think there's two options for how to do this: type Result struct {
Updated bool
StatusUpdated bool
} or type Result string
const (
ResultUnchanged Result = "unchanged"
ResultUpdated Result = "updated"
ResultUpdatedStatus Result = "updatedStatus"
ResultUpdatedAll Result = "updatedAll"
) I think I prefer the result struct rather than the result string, since it seems easier to use in conditional expressions that drive the client update calls. |
A new type for sure, and I'm fine with the struct approach (pretty similar to |
Signed-off-by: rashmigottipati <chowdary.grashmi@gmail.com>
Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
…ions Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
6d5a162
to
60ab6fd
Compare
Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
/test pull-controller-runtime-test-master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: estroz, joelanford, rashmigottipati The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@estroz @joelanford Can we in the future please either tell our contributors to squash their commits or use the label to have them squashed on merge? Something like this clutters the git history bigtimes for no good reason. |
@alvaroaleman yes, a worthwhile callout. |
@alvaroaleman, my bad I didn't realize that the PR would get automatically merged after it's approved. Will be mindful of that going forward. Thanks for mentioning it. |
My bad! Perhaps we should configure the merge bot to squash by default? I feel like I'm guilty of approving several PRs without using the label or asking for a squash because I'm used to other projects where that happens automatically. |
Closes #1453
PR for adding a finalizer helper library to controller-runtime
Signed-off-by: rashmigottipati chowdary.grashmi@gmail.com